use dataVolumeTemplates to perform image-based provisioning#178
use dataVolumeTemplates to perform image-based provisioning#178
Conversation
3d37370 to
0421ebb
Compare
62ecc15 to
d40037b
Compare
8ecafc0 to
5e35b8e
Compare
29de397 to
3d8f7e2
Compare
| record.create_vm({ :name => "test", :provision_method => 'image', :image_id => "default/template", :volumes_attributes => { "0" => { :capacity => "10", :bootable => "true" } }, :interfaces_attributes => { "0" => { "cni_provider" => "multus", "network" => "default/network" } } }) | ||
| end | ||
|
|
||
| test "raises an error for image based provisioning with only an extra data volume" do |
There was a problem hiding this comment.
this duplicates the "create_vm image based without additional volumes should fail" test below, so probably can be dropped now? (in a previous iteration of this PR this checked that a VM actually gets created correctly)
| record.create_vm({ :name => "test", :volumes_attributes => { 0 => { :capacity => "5" } }, :interfaces_attributes => { "0" => { "cni_provider" => "multus", "network" => "default/network" } } }) | ||
| end | ||
|
|
||
| test "raises an error for image based provisioning without an explicit boot volume" do |
There was a problem hiding this comment.
this duplicates the "create_vm image based without additional volumes should fail" test below, so probably can be dropped now? (in a previous iteration of this PR this checked that a VM actually gets created correctly)
There was a problem hiding this comment.
Is this still a case? Can't find the create_vm image based without additional volumes should fail test
There was a problem hiding this comment.
(in: test/unit/foreman_kubevirt_test.rb)
shubhamsg199
left a comment
There was a problem hiding this comment.
Tested and works as expected. Able to create a host via foreman and dataVolumeTemplates is used to create the said DataVolume
| record.create_vm({ :name => "test", :volumes_attributes => { 0 => { :capacity => "5" } }, :interfaces_attributes => { "0" => { "cni_provider" => "multus", "network" => "default/network" } } }) | ||
| end | ||
|
|
||
| test "raises an error for image based provisioning without an explicit boot volume" do |
There was a problem hiding this comment.
Is this still a case? Can't find the create_vm image based without additional volumes should fail test
| end | ||
|
|
||
| def add_volume_for_image_provision(options) | ||
| def add_volume_template_for_image_provision(options) |
There was a problem hiding this comment.
In the UI, when provisioning a host from an image, I need to add a bootable storage disk.
But that's something users don't know they are required to do.
Should we include it in the UI as a tooltip or at least mention it in the docs?
cc @jafiala
There was a problem hiding this comment.
how is that solved in other CRs? it's not like we're the first one to require a target when deploying something? :)
There was a problem hiding this comment.
answering myself, vm_instance_defaults:
foreman_kubevirt/app/models/foreman_kubevirt/kubevirt.rb
Lines 221 to 226 in 34e6a25
There was a problem hiding this comment.
would that make the storage class default for both image based and non image based right?
There was a problem hiding this comment.
In other CRs taking example of VMware, the volume section is already added with some default size and other default fields
There was a problem hiding this comment.
Yeah it would make the storage section be populated with one default volume for both provisioning methods, but it's also kinda right -- you can't provision without a (target) disk after all.
There was a problem hiding this comment.
Yeah agree , we need at least one disk to be selected but also which storage class to be setted as default is a concern , as there is also a way to define default storage class in kuberneties/openshift https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/ and we can also check if a storage class is default or not in https://github.com/fog/fog-kubevirt/blob/master/lib/fog/kubevirt/compute/models/storageclass.rb#L33 while parsing
There was a problem hiding this comment.
added the "empty" volume for now, we can enhance later
| raise ::Foreman::Exception.new _('VM should be created based on an image') unless image | ||
|
|
||
| verify_booting_from_image_is_possible(options[:volumes_attributes]) | ||
| namespace, name = image.split('/', 2) |
There was a problem hiding this comment.
When creating the image, I believe we should update the tooltip to include an option to specify whether to include a namespace. Also, if the token works in other workspaces that are not configured on the compute resource, it works there as well. This edge case could be mentioned in the docs only.
The API help text will need to be updated as well.
There was a problem hiding this comment.
Updated the help in the UI.
The API is not altered by the plugin (as the field comes from core), so can't be done there.
stejskalleos
left a comment
There was a problem hiding this comment.
My comments have been addressed, code looks good, but I haven't tested it with the latest changes.
@shubhamsg199, once you confirm the functionality, we can merge.

The current code uses
containerDiskfor image-based provisioning, but this results in an ephemeral disk being created on the fly, which is probably not what most users want.This PR changes it to use a
dataVolume, created using adataVolumeTemplate(see https://kubevirt.io/user-guide/storage/disks_and_volumes/#datavolume).While working on this, I realized that the current UI doesn't give a way to configure the resulting (root) volume when using image-based provisioning. So this PR also changes this flow a bit: a "bootable" volume needs to be configured, the details of that volume (capacity, storage class) are used for the
dataVolumeTempaltecreated one (previously, creating a "bootable" volume while doing image-based provisioning would result in an error).Test notes:
openshift-virtualization-os-images/centos-stream9-amd64on our CNV cluster, you can see a list of available ones withoc get datasources -n openshift-virtualization-os-imagesDependencies: